Skip to content

Fixes #5652. Execute timeout callbacks outside the queue lock - #5655

Merged
tig merged 14 commits into
tui-cs:developfrom
harder:fix/5652-timed-events-locking
Sep 14, 2026
Merged

tig merged 14 commits into
tui-cs:developfrom
harder:fix/5652-timed-events-locking

Conversation

@harder

@harder harder commented Aug 31, 2026

Copy link
Copy Markdown
Member

Summary

  • Execute timeout callbacks outside the timeout-queue lock so callbacks can add or remove timers and IApplication.Invoke can enqueue work from other threads. Because Invoke is implemented as TimedEvents.Add (TimeSpan.Zero, …), every cross-thread Invoke, Post, and Send previously blocked for the full duration of every timer callback.
  • Serialize callbacks with a reentrant runner gate. A competing RunTimers call returns, a later pass handles remaining due timers, and callback exceptions propagate directly and end the current pass.
  • Assign each queued timeout occurrence an identity and execute only occurrences already due when a timer pass starts. This prevents an immediately repeating timeout from spinning or consuming every pass slot ahead of an already-due peer.
  • Treat a reused Timeout reference as one cancellation token: Remove cancels all queued and active occurrences ordered before it without interrupting an executing callback. Removal generations and a StopAll epoch keep later additions unaffected without allocating per callback.
  • Scan the whole queue by reference in Remove and GetTimeout. Both previously used SortedList.IndexOfValue, which returns only the first match, so removing a Timeout that had been queued more than once left the remaining occurrences scheduled and still firing.
  • Cancel an in-flight repeating callback. Remove during execution was previously a silent no-op, and the callback rescheduled itself regardless.
  • Return a synchronized snapshot from Timeouts, validate Add(Timeout) inputs at the call site, and report the collision-adjusted queue key through Added.
  • Raise TimedEvents.Added after releasing the queue lock. A concurrent runner may execute a due timeout before its Added handler runs.
  • Evaluate virtual Timeout.Span getters and injected time providers outside the queue lock. Repeating occurrences remain active during these reads and revalidate their removal generation and StopAll epoch before rescheduling. Empty timer passes return before reading the provider.
  • Replace the orphaned <inheritdoc/> on CheckTimers, which is not declared on ITimedEvents and therefore inherited nothing.
  • Cover direct IApplication.Invoke and MainLoopSyncContext.Post/Send paths introduced by Fixes #5636 - Scope MainLoopSyncContext to running sessions (await-before-RunAsync deadlock) #5641. Send enqueues without the timeout-queue lock but still waits synchronously for main-loop execution by design.
  • Document the concurrency, cancellation, event-ordering, exception, and synchronous-dispatch contracts, including that an Added handler exception ends the timer pass while leaving the timeout scheduled.

SkillView consumer reproduction

SkillView 0.3 exposed the lock cycle in a real nested-modal workflow. A timeout-delivered UI callback opened the cleanup modal, then the removal worker tried to report its terminal progress through IApplication.Invoke. The captured stacks were:

UI:     CleanupScreen.Show → IApplication.Run → TimedEvents.RunTimersImpl
worker: RemoveService.BatchProgressAdapter.CompleteBatch → CallbackProgress.Report
        → CleanupScreen.InvokeIfActive → IApplication.Invoke → TimedEvents.AddTimeout → Monitor.Enter

The worker had completed two removal validations with refusals, but it could not enqueue the completion callback while the outer timer callback retained the queue lock. The modal therefore stayed on removing...; pressing Escape changed it to canceling removal..., but cancellation could not finish and the rest of the UI remained unavailable. CrossThread_Invoke_Executes_During_Nested_Run_Started_From_Timeout now covers this combined condition and verifies that the queued callback executes while the nested dialog is active.

Testing

  • Required CI: all build, documentation, performance, integration, and unit-test jobs passed on macOS, Ubuntu, and Windows
  • Targeted build: 0 errors and 3 pre-existing warnings — CS0419 in ViewBase/View.Drawing.cs and CS1574 ×2 in Views/DropDownList.cs. None are in files this PR touches, and the change introduces no new warnings.
  • TimedEventsTests: 40 passed
  • Nested-run tests: 6 passed
  • Timeout tests: 38 passed
  • Non-parallel tests: 72 passed, 2 skipped
  • Integration tests: 343 passed, with the known local GitVersion assembly-version failure
  • Full parallel run: 17,628 passed, 17 skipped, with 5 unrelated ANSI/color-output baseline failures

Verified behaviors

Beyond the assertions in the suite, these were confirmed with throwaway probes:

  • A repeating TimeSpan.Zero timeout runs exactly once per pass. Before the pass bound, it re-ran indefinitely — measured at ~17.4M invocations in 3 seconds with RunTimers never returning, which starves drawing, input, and shutdown.
  • A zero-span repeater no longer prevents an already-due peer from running in the same pass. Both zero-delay regression tests use lock-free stop signals so a broken pass bound fails cleanly instead of hanging during cleanup.
  • A throwing callback leaves no residue: the active-state map, the occurrence-ID map, and the queue all return to empty.
  • A throwing Added subscriber on the reschedule path leaves the active-state map empty and the queue and occurrence-ID map consistent; the timeout stays scheduled and later passes keep running it.
  • Blocking Timeout.Span overrides and time-provider reads no longer block concurrent timeout removal or addition; cancellation during a repeat-interval read still prevents rescheduling.

Review notes

Draft pending final review. The normal local GitVersion step hung in GitVersion.MsBuild on one machine; the equivalent build with DisableGitVersionTask=true passed, as does a plain dotnet build. Solution builds emit only the pre-existing compiler warnings listed above plus sandbox-related NuGet audit-cache warnings (NU1900).

To pull down this PR locally:

git remote add copilot https://github.com/harder/Terminal.Gui.git
git fetch copilot fix/5652-timed-events-locking
git checkout copilot/fix/5652-timed-events-locking

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Per-token cancellation bookkeeping can cancel newly added occurrences while allowing pre-existing active occurrences to reschedule.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Moves timeout callbacks outside the queue lock while preserving serialized execution and cancellation behavior.

Changes:

  • Adds a reentrant runner gate and pending-run handoff.
  • Tracks active/cancelled repeating timeouts.
  • Adds concurrency and synchronization-context regression tests.
File summaries
File Description
TimedEvents.cs Refactors timer execution and cancellation locking.
TimedEventsTests.cs Adds concurrency and deadlock regression coverage.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Terminal.Gui/App/Timeout/TimedEvents.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Pending follow-up callback exceptions can be silently discarded.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

Terminal.Gui/App/Timeout/TimedEvents.cs:133

  • When a pending follow-up pass also throws after an earlier callback failed, ??= silently discards the later exception even though that timeout has already been dequeued. No caller can observe or retry that failure; accumulate subsequent failures (for example, in an AggregateException) before rethrowing.
                catch (Exception ex)
                {
                    error ??= System.Runtime.ExceptionServices.ExceptionDispatchInfo.Capture (ex);
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The core timer scheduler now relies on intricate cross-thread handoff and reentrant cancellation behavior requiring final human review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

harder and others added 2 commits August 31, 2026 21:09
Address review nits on the timer cancellation rework:

- Add a deterministic test for the documented contract that Remove
  returns true for an occurrence that has already been dequeued and is
  executing, and neither waits for nor interrupts that callback. This
  was documented on ITimedEvents.Remove but not covered by a test.
- Note that ActiveTimeoutState is a mutable struct in a Dictionary, so
  every mutation must be written back before the queue lock is released.
- Document ActiveTimeoutOccurrence and ActiveTimeoutState.
- Note that Remove intentionally scans the whole queue, because the same
  Timeout instance can be queued more than once.
- Correct the class-level thread-safety docs. The blanket "Thread-safe
  for concurrent access" claim was inaccurate: Timeouts returns the live
  queue rather than a snapshot and is not synchronized.
- Replace the orphaned <inheritdoc/> on CheckTimers with real docs.
  CheckTimers is not declared on ITimedEvents, so it inherited nothing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Core timer concurrency and cancellation semantics warrant final human review despite extensive regression coverage.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Prevent zero-delay repeating timeouts from starving the main loop by limiting each pass to the number of callbacks due at its start.

Validate Timeout inputs, return a synchronized queue snapshot, and report the collision-adjusted Added timestamp with deterministic regression coverage.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Immediate rescheduling can let one repeating timeout consume the pass budget and indefinitely starve other due timers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1169

  • This AI-test marker omits the required separator. The accepted forms are documented in .claude/rules/testing-patterns.md:15-22.

This issue also appears in the following locations of the same file:

  • line 1178
  • line 1189
  • line 1205
  • line 1220

Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1178

  • This AI-test marker omits the required separator. The accepted forms are documented in .claude/rules/testing-patterns.md:15-22.
    // Claude Opus 5

Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1189

  • This AI-test marker omits the required separator. The accepted forms are documented in .claude/rules/testing-patterns.md:15-22.
    // Claude Opus 5

Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1205

  • This AI-test marker omits the required separator. The accepted forms are documented in .claude/rules/testing-patterns.md:15-22.
    // Claude Opus 5

Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1220

  • This AI-test marker omits the required separator. The accepted forms are documented in .claude/rules/testing-patterns.md:15-22.
    // Claude Opus 5
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread Terminal.Gui/App/Timeout/TimedEvents.cs Outdated
Assign each queued timeout occurrence a monotonic identity and limit a timer pass to occurrences that were already queued and due when it began.

Add a deterministic zero-delay repeater plus peer regression and correct the AI test markers noted by review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The intricate concurrency and cancellation state machine warrants final human review despite strong regression coverage.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

CompleteTimeout runs from the finally that follows a timeout callback, and it
raised Added. That made an Added subscriber the only user-code call site
reachable from that finally, so a throwing subscriber could replace an exception
raised by the callback itself.

The masking was not reachable in practice: a throwing callback leaves repeat
false, so CompleteTimeout took its early-return path, which only reads the
active-state entry, decrements it, and either removes it or assigns over an
existing key. None of that allocates, so none of it could throw. The guard was
incidental rather than designed, though, and any future change that let repeat be
true on an error path would have turned it into real masking.

Return the reschedule result from CompleteTimeout instead and raise Added from
the timer loop once the callback has returned normally. Bookkeeping and
rescheduling stay in the finally, still under the queue lock, so the ordering
Remove depends on is unchanged, and Added is still raised after that lock is
released.

Document the resulting contract: an Added handler exception ends the timer pass
while leaving the timeout scheduled, so a handler that throws on every reschedule
ends every subsequent pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

One claimed exception-ordering regression test never exercises its Added handler.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs Outdated
A throwing timeout callback cannot also request rescheduling because the callback result is never assigned. The Added event is therefore unreachable on that path, so the exception-masking regression did not exercise the behavior it claimed.

Remove the unsupported regression and restore the simpler occurrence-completion flow. Keep the separate coverage for the reachable case where an Added handler throws after a repeating timeout has been rescheduled.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Timer selection can degrade quadratically with many immediately repeating timeouts, and wrapper documentation remains inconsistent.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

Terminal.Gui/App/Timeout/TimedEvents.cs:407

  • This rescans deferred occurrences from index 0 after every callback. With N same-time zero-span repeaters, each reschedule reuses a low queue key but receives an ID above the cutoff, so the next lookup skips 1, then 2, … deferred entries—making one pass O(N²) (1,000 repeaters require about 501,500 scans). This can stall the UI timer pass; capture the eligible (key, occurrenceId) entries once per pass, or otherwise keep a cursor/index that does not repeatedly traverse deferred entries.
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread Terminal.Gui/App/Timeout/ITimedEvents.cs
Align IApplication.RemoveTimeout with the ITimedEvents.Remove contract. A true result can mean cancellation was recorded for an executing callback, which continues running but cannot reschedule.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Repeat rescheduling still evaluates a virtual property while holding the timeout queue lock, permitting lock-inversion deadlocks.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread Terminal.Gui/App/Timeout/TimedEvents.cs Outdated
Read a repeating Timeout Span and the current time before reentering the timeout queue lock, then revalidate the occurrence generation and StopAll epoch before enqueueing. Preserve active-state cleanup if either extension point throws.

Apply the same lock boundary to GetTimeout and all time-provider reads. Add deterministic regressions for blocking and throwing Span overrides plus blocking time providers.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

RunTimers now unnecessarily reads potentially blocking user-provided time when no timeouts are queued.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread Terminal.Gui/App/Timeout/TimedEvents.cs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The zero-delay repeater regression can hang indefinitely when the behavior it tests regresses.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1530

  • If the zero-delay repeater regresses to the previous unbounded pass, the runner retains the queue lock and this synchronous Remove blocks forever, so the regression test hangs instead of failing within its timeout. Stop the loop by replacing the callback first, wait for the runner with the existing bound, and then clean up the queued occurrence.
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@harder

harder commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Addressed the latest Copilot review summary in 8de9e59. The zero-delay pass-bound regression now uses an atomic stop signal before waiting for the runner, so a regression terminates and fails its bounded-return assertion instead of blocking in Remove. I applied the same hardening to the adjacent zero-delay peer-starvation regression and preserved its assertion that the repeating occurrence remains queued. TimedEventsTests passes 40/40 locally.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The intricate concurrency and cancellation-state changes warrant final human review despite strong regression coverage.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@harder
harder marked this pull request as ready for review September 3, 2026 02:35
@harder
harder requested a review from tig as a code owner September 3, 2026 02:35

@tig tig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lovely. thanks.

@tig
tig merged commit 8b986b1 into tui-cs:develop Sep 14, 2026
14 checks passed
@harder
harder deleted the fix/5652-timed-events-locking branch September 14, 2026 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TimedEvents.RunTimers executes callbacks while holding the timeout queue lock

3 participants